feat(loans): native amortization schedule for fixed-rate loans (#1804)#1887
feat(loans): native amortization schedule for fixed-rate loans (#1804)#1887Rene0422 wants to merge 4 commits into
Conversation
superagent-security
Bot
added
contributor:verified
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a memoized fixed-rate amortization schedule with aggregation helpers and payoff_date to Loan, displays total interest/payoff and a collapsible amortization table in the loan overview, introduces locale keys for schedule labels, and adds tests verifying schedule correctness and edge cases. ChangesLoan Amortization Schedule Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49216c4829
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <details class="mt-6 rounded-lg border border-primary bg-container"> | ||
| <summary class="cursor-pointer select-none px-4 py-3 text-sm font-medium text-primary"> | ||
| <%= t(".amortization_schedule") %> |
There was a problem hiding this comment.
This view introduces a custom <details>/<summary> disclosure instead of the design-system primitive, which violates the repository rule in /workspace/sure/AGENTS.md (“Reach for DS::* first” and specifically lists DS::Disclosure). Shipping bespoke disclosure markup here means behavior/styling/accessibility fixes made in DS::Disclosure will not propagate to loans, and this guideline is explicitly marked as a close/rewrite-level issue for reviewers.
Useful? React with 👍 / 👎.
| if period == term_months || principal_amount > balance | ||
| principal_amount = balance.round(2) | ||
| end | ||
|
|
||
| payment_amount = (interest_amount + principal_amount).round(2) |
There was a problem hiding this comment.
When rounding causes the balance to hit zero before the final term, the loop keeps emitting remaining periods with $0 payment/interest/principal instead of ending the schedule. Because payoff_date is taken from the last row, this can push the reported payoff later than the actual payoff month (e.g., small principals with long terms, such as balance=1000, interest_rate=9.4, term_months=360, reach zero at period 359 but still add period 360 as all zeros).
Useful? React with 👍 / 👎.
Rene0422
force-pushed
the
feat/loan-amortization-schedule
branch
from
4bc8578 to
c00a8f8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/models/loan_test.rb`:
- Around line 34-35: The test currently does an exact float equality
assert_equal on the computed balance using first[:beginning_balance].amount,
first[:principal].amount, and first[:ending_balance].amount which can be flaky;
change the assertion in test/models/loan_test.rb to use assert_in_delta with a
small tolerance (e.g. 0.01 or 1e-6) comparing
(first[:beginning_balance].amount.to_f - first[:principal].amount.to_f) and
first[:ending_balance].amount.to_f so tiny floating-point rounding differences
won't fail the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5e908fb-f013-4843-9b3b-8e36f8917764
📒 Files selected for processing (4)
app/models/loan.rbapp/views/loans/tabs/_overview.html.erbconfig/locales/views/loans/en.ymltest/models/loan_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/loans/en.yml
|
Nice feature, good test coverage. A few things worth discussing before merge: 1. Floating-point arithmetic for financial calculations. 2. Schedule start date produces historical dates. 3. Raw 4. Performance on long schedules. The schedule is memoized per instance (good for a single request), but 360-row generation happens on every page load for a 30-year loan. This is fine at current scale, but if this is called in list views or anywhere the Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/views/loans/tabs/_overview.html.erb (1)
63-65: ⚡ Quick winMove the upcoming-row filtering out of the template.
Selecting future rows with
Date.currentis domain/data-shaping logic in the view. Expose anupcoming_amortization_schedulemethod/helper and keep this partial focused on rendering.As per coding guidelines, "Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/loans/tabs/_overview.html.erb` around lines 63 - 65, The view currently performs domain/data-shaping by computing upcoming_schedule via account.loan.amortization_schedule.select { |row| row[:payment_date] >= Date.current }; move that filtering into a helper or model method (e.g., define upcoming_amortization_schedule on the Loan model or a view helper) and replace the inline select in the partial with a call to that method (use the new upcoming_amortization_schedule or helper name), keeping the partial responsible only for rendering and not for date-based selection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/loan.rb`:
- Around line 88-89: The monthly payment and schedule are computed on two
different code paths causing cent-level mismatches; update the implementation so
one method delegates to the other — e.g., change Loan#monthly_payment to call
exact_monthly_payment(principal, monthly_rate) (or have
build_amortization_schedule call monthly_payment) so both use the same
calculation and rounding logic; ensure you compute monthly_rate the same way
(interest_rate.to_d / 100).div(12, DIVISION_PRECISION) and reuse that value, and
apply this same delegation fix for the related methods around the other
occurrence (lines handling the schedule calculation) so summary and schedule
always match.
---
Nitpick comments:
In `@app/views/loans/tabs/_overview.html.erb`:
- Around line 63-65: The view currently performs domain/data-shaping by
computing upcoming_schedule via account.loan.amortization_schedule.select {
|row| row[:payment_date] >= Date.current }; move that filtering into a helper or
model method (e.g., define upcoming_amortization_schedule on the Loan model or a
view helper) and replace the inline select in the partial with a call to that
method (use the new upcoming_amortization_schedule or helper name), keeping the
partial responsible only for rendering and not for date-based selection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 815def02-4c85-47da-99e1-664b1168802e
📒 Files selected for processing (2)
app/models/loan.rbapp/views/loans/tabs/_overview.html.erb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/loan_test.rb (1)
32-32: 💤 Low valueConsider using
assert_in_deltafor consistency with other float assertions.Line 32 uses exact equality on a float, while lines 33-35 correctly use
assert_in_delta. Although 1200.0 is exactly representable in IEEE 754, using the delta pattern consistently makes the test suite more uniform and prevents copy-paste issues for future tests with non-exact values.Suggested change
- assert_equal 1200.00, first[:interest].amount.to_f + assert_in_delta 1200.00, first[:interest].amount.to_f, 0.01🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/loan_test.rb` at line 32, Replace the exact-float assertion with a delta-based assertion: change the assertion that compares first[:interest].amount.to_f to 1200.00 from assert_equal to assert_in_delta and supply the same small delta used elsewhere in this test file (e.g., 0.001) so it reads like: assert_in_delta 1200.00, first[:interest].amount.to_f, 0.001.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/models/loan_test.rb`:
- Line 32: Replace the exact-float assertion with a delta-based assertion:
change the assertion that compares first[:interest].amount.to_f to 1200.00 from
assert_equal to assert_in_delta and supply the same small delta used elsewhere
in this test file (e.g., 0.001) so it reads like: assert_in_delta 1200.00,
first[:interest].amount.to_f, 0.001.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 099523d1-a85d-41d4-8207-ec1dbe6a0704
📒 Files selected for processing (2)
app/models/loan.rbtest/models/loan_test.rb
2 participants
Summary
Implements the schedule half of #1804. Fixed-rate loan accounts now expose a French-style amortization schedule, total interest, total payments, and projected payoff date — surfaced as two new summary cards and a collapsible table on the loan's Overview tab.
Closes the data/visibility gap users hit today: they no longer need an external amortization table to see how each monthly payment splits between principal and interest.
What's included
Loan#amortization_schedule— array of{ period, payment_date, beginning_balance, payment, interest, principal, ending_balance }, memoized per instanceLoan#total_interest,#total_payments,#payoff_date[]/nilfor non-fixed rate loans, missingterm_months/interest_rate, or zero principal<details>schedule table (Hotwire-first, no JS)en.yml; non-English locales fall back viaconfig.i18n.fallbackstest/models/loan_test.rbWhat's deliberately not included
Auto-splitting incoming loan payments into a principal transfer + interest expense — the other half of #1804. That change touches
Transactioncreation, provider sync (Plaid / SimpleFIN), idempotency, and undo/edit semantics around extra-principal payments. It warrants its own design discussion and PR. Filed as a follow-up.Test plan
bin/rails test test/models/loan_test.rbpassesbin/rails testpassesbin/rubocop -f github -acleanbundle exec erb_lint ./app/**/*.erb -acleanbin/brakeman --no-pagerclean<details>table expands to 360 rows for a 30-year loan; final-row Balance is$0.00$0interest column and equal principal across all rowsSummary by CodeRabbit